bugfix(contain): Restore retail compatibility after crash fix in OpenContain::processDamageToContained by Caball009 · Pull Request #2427 · TheSuperHackers/GeneralsGameCode

Caball009 · 2026-03-08T17:59:12Z

Closes Restore retail compatibility after crash fix in OpenContain::processDamageToContained #2223
Follow up for [GEN][ZH] Fix crash in OpenContain::processDamageToContained() when killed occupants kill the host container #1019

The way the crash fix was implemented in #1019 assumes the container size is either unchanged or 0. This is not the case in the attached replay in #2223.

Gameplay without this PR:

processDamageToContained_before2.mp4

Gameplay with this PR:

processDamageToContained_after2.mp4

Due to a different bug, a GLA Worker enters the unmanned Emperor Overlord but becomes part of the crew instead of the pilot. When the Emperor gets destroyed it first removes its Gattling turret and so the container size has changed. The Worker is still inside, though, so breaking out of the loop is not allowed.

TODO:

Replicate in Generals.
Polish comments.

greptile-apps · 2026-03-08T18:03:55Z

Greptile Summary

This PR fixes a retail-compatibility regression introduced by PR #1019, where the previous crash-fix used a break when the container list size changed — an assumption that only held when the list was fully cleared (e.g. the GLA Battle Bus scenario). In the newly discovered case (Emperor Overlord with a mixed crew), the container shrinks by one element mid-iteration while still having remaining occupants, causing the old code to prematurely stop processing them.

Key changes:

Replaces the size-change-based break/continue logic in the RETAIL_COMPATIBLE_CRC path with a snapshot copy of m_containList taken before iteration begins (stack buffer for small containers < 16, std::vector for larger ones).
Extracts the shared damage loop into a new processDamageToContainedInternal(Object* const*, size_t, Real) helper to avoid code duplication between the two copy paths.
Updates the #else (non-retail) path's comment to use consistent language with the new implementation.
Adds a DEBUG_ASSERTCRASH to validate the m_containListSize invariant at the start of the retail path.

The copy-based approach is the correct solution agreed upon in previous review threads: iterating a snapshot means any modification to m_containList during damage processing cannot invalidate the iterator or cause premature loop exit.

Confidence Score: 5/5

Safe to merge; the fix is logically sound and correctly addresses the regression without breaking retail CRC compatibility.

No P0 or P1 issues found. Both findings are P2 style/robustness suggestions (access specifier placement and a minor size-source inconsistency guarded by a DEBUG_ASSERTCRASH). The core fix — iterating over a snapshot copy — is the approach agreed on in prior review threads and is correctly implemented.

No files require special attention; both findings are non-blocking.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Include/GameLogic/Module/OpenContain.h	Adds `processDamageToContainedInternal` declaration under `RETAIL_COMPATIBLE_CRC`; helper is placed in the public section but should be protected/private.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	Replaces the old size-change-based `break` logic with a snapshot copy (stack for <16, heap for ≥16), fixing premature loop exit when container size changes mid-iteration; minor size-source inconsistency between the two copy paths.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[processDamageToContained] --> B{RETAIL_COMPATIBLE_CRC?}

    B -- Yes --> C[Assert m_containListSize == m_containList.size]
    C --> D{m_containListSize < 16?}
    D -- Yes --> E["Stack copy: Object*[16]\nstd::copy from m_containList"]
    D -- No --> F["Heap copy: std::vector<Object*>\nconstructed from m_containList"]
    E --> G[processDamageToContainedInternal\nsize = m_containListSize]
    F --> H[processDamageToContainedInternal\nsize = containCopy.size]
    G --> I[Iterate snapshot safely]
    H --> I

    B -- No --> J[Swap m_containList into local list\nm_containListSize = 0]
    J --> K[Iterate local list applying damage]
    K --> L[Swap list back into m_containList]

Prompt To Fix All With AI

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Include/GameLogic/Module/OpenContain.h
Line: 222-224

Comment:
**`processDamageToContainedInternal` placed in public section**

`processDamageToContainedInternal` is a pure implementation-detail helper for `processDamageToContained`. It sits in the `public:` section (the `protected:` block starts at line 235), making it callable by unrelated code. It should be moved under `protected:` or `private:` to enforce encapsulation.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp
Line: 1512-1517

Comment:
**Stack path passes `m_containListSize`; heap path passes `containCopy.size()`**

The two paths pass different size values to `processDamageToContainedInternal`:

- **Stack path** (lines 1512–1517): `std::copy` copies `m_containList.size()` elements, but the count given to the internal function is `m_containListSize`.
- **Heap path** (lines 1519–1523): the count is derived from the vector (`containCopy.size()`), which equals `m_containList.size()`.

In debug builds the `DEBUG_ASSERTCRASH` at line 1505 ensures `m_containListSize == m_containList.size()`, so both paths are equivalent there. In a release build, if the invariant is ever broken, the stack path could silently skip occupants or read past the end of `containCopy`. For consistency and defensive robustness, both paths should derive their count from the same source (`m_containList.size()`).

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (6): Last reviewed commit: "Addressed feedback." | Re-trigger Greptile}

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp

Caball009 · 2026-03-20T16:04:14Z

I reset the branch and broke the new diff into manageable commits.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp

xezon

If it work, it works.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp

Caball009 · 2026-03-27T16:35:11Z

Addressed feedback.

In the unlikely scenario that the new implementation turns out not to be fully retail compatible, I expect that adding the following code here would fix it:

if (size != m_containListSize)
{
	if (m_containListSize == 0)
		break;

	if (object->isEffectivelyDead())
		continue;

	if (std::find(m_containList.begin(), m_containList.end(), object) == m_containList.end())
		continue;
}

Caball009 added Bug Something is not working right, typically is user facing Major Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour ThisProject The issue was introduced by this project, or this task is specific to this project labels Mar 8, 2026

greptile-apps bot reviewed Mar 8, 2026

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp Outdated Show resolved Hide resolved

Caball009 marked this pull request as draft March 10, 2026 19:29

Caball009 force-pushed the fix_OpenContain_processDamageToContained branch from b533974 to b342a9f Compare March 11, 2026 08:33

Caball009 marked this pull request as ready for review March 11, 2026 08:51

greptile-apps bot reviewed Mar 11, 2026

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp Outdated Show resolved Hide resolved

Mauller reviewed Mar 12, 2026

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp Outdated Show resolved Hide resolved

xezon reviewed Mar 17, 2026

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp Outdated Show resolved Hide resolved

Caball009 added 7 commits March 20, 2026 16:28

Added 'processDamageToContainedInternal' for refactor.

0dcb53d

Moved loop to internal function.

6e5d403

Added call to internal function & cleanup.

38dbc74

Changed comments.

91ac7d0

Added list copy logic.

c64c9a1

Added optimization for small list size.

144e6b4

Hoisted 'isBurnedDeathToUnits' outside of the loop.

4d5d684

Caball009 force-pushed the fix_OpenContain_processDamageToContained branch from 6c35643 to 4d5d684 Compare March 20, 2026 15:46

Caball009 commented Mar 20, 2026

View reviewed changes

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp Outdated Show resolved Hide resolved

Changed comments.

cebb0a5

xezon approved these changes Mar 23, 2026

View reviewed changes

Addressed feedback.

6c196ce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix(contain): Restore retail compatibility after crash fix in OpenContain::processDamageToContained#2427

bugfix(contain): Restore retail compatibility after crash fix in OpenContain::processDamageToContained#2427
Caball009 wants to merge 9 commits intoTheSuperHackers:mainfrom
Caball009:fix_OpenContain_processDamageToContained

Caball009 commented Mar 8, 2026 •

edited

Loading

Uh oh!

greptile-apps bot commented Mar 8, 2026 •

edited

Loading

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caball009 commented Mar 20, 2026

Uh oh!

Uh oh!

xezon left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caball009 commented Mar 27, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Caball009 commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Gameplay without this PR:

Gameplay with this PR:

TODO:

Uh oh!

greptile-apps bot commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caball009 commented Mar 20, 2026

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caball009 commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Caball009 commented Mar 8, 2026 •

edited

Loading

greptile-apps bot commented Mar 8, 2026 •

edited

Loading

Caball009 commented Mar 27, 2026 •

edited

Loading